feat(substrait): serialize correlated subqueries with OuterReference field references#23488
feat(substrait): serialize correlated subqueries with OuterReference field references#23488clflushopt wants to merge 3 commits into
Conversation
|
The inception for this PR came from a discussion I had with @vbarua on Datafusion's Discord; do note that initially this was marked as a bit tricky to do and would warrant a discussion so looking back at the history specifically #18987 and its consumer half in #20439 I found a good middle ground to attempt an approach. Open to change or even close and discuss. |
bvolpato
left a comment
There was a problem hiding this comment.
Direction and implementation look good. Standard OuterReference encoding matches #20439, and local Substrait suite passed.
PR description starts with unrelated SingleDistinctToGroupBy content. Could you remove that copied section?
Wow that threw me off for a bit I was looking at a existing PRs to understand the PR template I should follow and I must have accidently copy-paste it into this one. Fixed. |
vbarua
left a comment
There was a problem hiding this comment.
Seems pretty reasonable overall from the Substrait perspective. Pushing and popping schemas for query scopes makes sense for dealing with outer references.
| pub fn from_outer_reference_column( | ||
| producer: &mut impl SubstraitProducer, | ||
| _field: &FieldRef, |
There was a problem hiding this comment.
What do we need _field for?
| col: &Column, | ||
| schema: &DFSchemaRef, | ||
| _schema: &DFSchemaRef, |
There was a problem hiding this comment.
Do we need _schema now that we're retrieving the schema from the outer reference stack? Given that we're already introducing producer as a breaking change here, it would make sense to remove _schema if it's not needed anymore.
| /// exceeds the current nesting depth. | ||
| fn get_outer_schema(&self, _steps_out: usize) -> Option<DFSchemaRef> { | ||
| None | ||
| } |
There was a problem hiding this comment.
Would it make sense to use not_impl_err! for the push, pop and get functions similar to how it was done for lambdas? This would make custom producers missing these methods fail more loudly.
There was a problem hiding this comment.
I believe it would break existing custom producers that serialize substraits because it would be invoked on every substrait boundary. The choice of defaulting to noops None is to match the consumer side as well which has the same default behaviour.
| /// `steps_out = 1` is the immediately enclosing query, `steps_out = 2` | ||
| /// is two levels out, etc. Returns `None` if `steps_out` is 0 or | ||
| /// exceeds the current nesting depth. | ||
| fn get_outer_schema(&self, _steps_out: usize) -> Option<DFSchemaRef> { |
There was a problem hiding this comment.
minor/opinionated: get_outer_schema -> outer_schema to better match lamba_variable (no get)
There was a problem hiding this comment.
I agree on this, I don't like the get but I was opting to match the api of SubstraitConsumer::get_outer_schema. I can do a separate micro PR to align both apis.
| substrait_field_ref(index) | ||
| let mut steps_out = 1; | ||
| while let Some(outer_schema) = producer.get_outer_schema(steps_out) { | ||
| if let Some(index) = outer_schema.maybe_index_of_column(col) { |
There was a problem hiding this comment.
This effectively searches the innermost schemas outwards searching for the first instance of a column matching col. I can imagine some weirdness if you have ambiguous references to a column with the same qualified name in multiple scopes. Does DataFusion enforce any requirements about having these names be unambiguous in plans?
There was a problem hiding this comment.
I don't think so I believe it bounds to the nearest enclosing scope, which I confirmed by adding a test for this. To ease review, below is what the test reproduces when printing out the outputs of each step.
SELECT b FROM data WHERE EXISTS (SELECT 1 FROM data WHERE EXISTS (SELECT 1 FROM book WHERE book.isbn = data.a))
- Datafusion's plan (unoptimized):
Projection: data.b
Filter: EXISTS (<subquery>)
Subquery:
Projection: Int64(1)
Filter: EXISTS (<subquery>)
Subquery:
Projection: Int64(1)
Filter: book.isbn = outer_ref(data.a)
TableScan: book
TableScan: data
TableScan: data
- Substrait's plan :
{
"version": {
"minorNumber": 85,
"producer": "datafusion"
},
"extensions": [
{
"extensionFunction": {
"extensionUrnReference": 4294967295,
"name": "equal"
}
}
],
"relations": [
{
"root": {
"input": {
"project": {
"common": {
"emit": {
"outputMapping": [
6
]
}
},
"input": {
"filter": {
"input": {
"read": {
"baseSchema": {
"names": [
"a",
"b",
"c",
"d",
"e",
"f"
],
"struct": {
"types": [
{
"i64": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"decimal": {
"scale": 2,
"precision": 5,
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"date": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"bool": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"i32": {
"typeVariationReference": 1,
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"string": {
"nullability": "NULLABILITY_NULLABLE"
}
}
],
"nullability": "NULLABILITY_REQUIRED"
}
},
"namedTable": {
"names": [
"data"
]
}
}
},
"condition": {
"subquery": {
"setPredicate": {
"predicateOp": "PREDICATE_OP_EXISTS",
"tuples": {
"project": {
"common": {
"emit": {
"outputMapping": [
6
]
}
},
"input": {
"filter": {
"input": {
"read": {
"baseSchema": {
"names": [
"a",
"b",
"c",
"d",
"e",
"f"
],
"struct": {
"types": [
{
"i64": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"decimal": {
"scale": 2,
"precision": 5,
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"date": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"bool": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"i32": {
"typeVariationReference": 1,
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"string": {
"nullability": "NULLABILITY_NULLABLE"
}
}
],
"nullability": "NULLABILITY_REQUIRED"
}
},
"namedTable": {
"names": [
"data"
]
}
}
},
"condition": {
"subquery": {
"setPredicate": {
"predicateOp": "PREDICATE_OP_EXISTS",
"tuples": {
"project": {
"common": {
"emit": {
"outputMapping": [
3
]
}
},
"input": {
"filter": {
"input": {
"read": {
"baseSchema": {
"names": [
"isbn",
"title",
"genre"
],
"struct": {
"types": [
{
"i64": {
"nullability": "NULLABILITY_REQUIRED"
}
},
{
"string": {
"nullability": "NULLABILITY_NULLABLE"
}
},
{
"string": {
"nullability": "NULLABILITY_NULLABLE"
}
}
],
"nullability": "NULLABILITY_REQUIRED"
}
},
"namedTable": {
"names": [
"book"
]
}
}
},
"condition": {
"scalarFunction": {
"arguments": [
{
"value": {
"selection": {
"directReference": {
"structField": {}
},
"rootReference": {}
}
}
},
{
"value": {
"selection": {
"directReference": {
"structField": {}
},
"outerReference": {
"stepsOut": 1
}
}
}
}
],
"outputType": {
"bool": {
"nullability": "NULLABILITY_NULLABLE"
}
}
}
}
}
},
"expressions": [
{
"literal": {
"i64": "1"
}
}
]
}
}
}
}
}
}
},
"expressions": [
{
"literal": {
"i64": "1"
}
}
]
}
}
}
}
}
}
},
"expressions": [
{
"selection": {
"directReference": {
"structField": {
"field": 1
}
},
"rootReference": {}
}
}
]
}
},
"names": [
"b"
]
}
}
]
}
…field references The Substrait producer previously errored on Expr::OuterReferenceColumn, making it impossible to serialize unoptimized plans containing correlated subqueries. The consumer already resolves OuterReference field references against a stack of outer schemas; this adds the producing half: - SubstraitProducer gains push_outer_schema/pop_outer_schema/get_outer_schema (mirroring SubstraitConsumer) and a handle_outer_reference_column hook. - DefaultSubstraitProducer maintains the outer-schema stack. - The subquery producers (InSubquery, ScalarSubquery, Exists, SetComparison) push the enclosing schema around the subquery plan. - from_outer_reference_column resolves the column against the stack, innermost first, and emits a FieldReference with an OuterReference root and the corresponding steps_out, per the Substrait spec.
…_column test(substrait): shadowed outer column resolves to nearest enclosing scope from review comments when the same qualified column name exists in multiple enclosing scopes, the innermost-out search binds to the nearest one, matching SQL scoping and SqlToRel's own resolution. The test asserts the producer emits steps_out = 1 and never 2 for a doubly-nested subquery whose two enclosing scopes both scan the same table.
b7ff0af to
6c0f568
Compare
Which issue does this PR close?
Rationale for this change
The Substrait producer errors on
Expr::OuterReferenceColumn, so any plan containing a correlated subquery cannot be serialized currently. DataFusion's round-trip tests don't hit this because the optimizer decorrelates subqueries into joins before serialization, but any workflow that serializes unoptimized plans (e.g. sending raw plans between systems for later optimization in my case) fails on queries like TPC-H q2/q4/q17/q20/q21/q22, and ~26 cases injoins.sltfail in--substrait-round-tripmode.A previous attempt (#18987) was closed because it introduced a non-standard mechanism for outer references, producing plans only DataFusion would be able consume but Substrait already represents correlated references natively via a
FieldReferencewith anOuterReferenceroot type and asteps_outdepth. The consumer side of was implemented in #20439, which resolvesOuterReferencefield references against a stack of outer schemas. This PR implements the producing half, symmetric with that design, so correlated plans round-trip using only standard Substrait.What changes are included in this PR?
SubstraitProducergains outer-schema-stack methods mirroringSubstraitConsumer:push_outer_schema/pop_outer_schema(default no-ops) andget_outer_schema(steps_out)(defaultNone), plus ahandle_outer_reference_columnmethod so custom producers can override the behaviour like every other expression kind. Defaults are backward compatible: existing custom producers are unaffected unless a plan actually contains an outer reference, in which case they now get an actionable error instead ofnot_impl_err.DefaultSubstraitProducermaintains the stack in aVec<DFSchemaRef>.from_in_subquery,from_scalar_subquery,from_exists,from_set_comparison) push the enclosing query's schema around the subquery plan conversion (via a sharedproduce_subquery_rel, analogous to the consumer'sconsume_subquery_rel).from_outer_reference_column(previously unused and emitting an incorrect plainRootReference) now resolves the column against the outer-schema stack, innermost first, and emits aFieldReferencewith anOuterReferenceroot and the correspondingsteps_out.to_substrait_rexdispatchesExpr::OuterReferenceColumnto the new handler instead of erroring.Are these changes tested?
Yes:
roundtrip_logical_plan.rscovering correlatedEXISTS, correlatedINsubquery, correlated scalar subquery, and a nested correlated subquery that crosses two subquery boundaries (steps_out = 2). Each test asserts the produced plan contains anOuterReferenceat the expected depth and that the plan round-trips through the existing consumer with its schema intact.joins.sltin--substrait-round-tripmode goes from 38 failures to 12; the remaining failures are pre-existing gaps unrelated to outer references (USINGjoin constraint, plan-level lateralLogicalPlan::Subquery, duplicate unqualified field names).Are there any user-facing changes?
OuterReferencefield references.SubstraitProducerhas three new provided methods andhandle_outer_reference_column; all have defaults, so existing implementations continue to compile.from_outer_reference_columnchanged (it now takes the producer and the outer field) — its previous form resolved against the wrong schema and emitted a plainRootReference, and it was not called from anywhere in the crate.